You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This introduces ZipperBuffer. This allows re-using zipper path and stack Vecs.
Name up for change, don't have a strong preference.
fnReadZipper::detach(self) -> ZipperBuffer;// saves pathfnZipperBuffer::attach(self,&map) -> ReadZipper;// restores pathfnReadZipper::relocate(self,&map) -> ReadZipper;// same path on a different map
detach + attach, relocate preserve the path. might need to change the API so this is explicit.
A few additional non-public API changes to make it work. descend_to_byte used instead of descend_to to avoid aliased &mut.
Nice! I quite like the API and naming.
The ancestor_node thing is a little ugly, but the lifetime tagging makes sense.
What's the motivation for going for a new object, rather than just a "disabled RZ" which is a zero-overhead opaque-type wrapper of a RZ?
I'm ok with the API concept. I'd prefer different names for detach and attach. And I think relocate has some issues.
detach doesn't say enough about what it does. It sounds too fundamental for an optimization like this. We could use the rust idiom for a consuming conversion, i.e. into_buffer or into_zipper_buffer (I prefer the latter). Another option is to make it clear we're destroying the zipper but saving the buffers with salvage_buffer. I think I like into_zipper_buffer.
On attach, same issue. That verb sounds really central to what it does conceptually. Which it isn't. My preference would be variants of PathMap::read_zipper_with_buffer(&self, buffer: ZipperBuffer<V, A>).
ZipperBuffer::into_read_zipper(map: &'map PathMap<V, A>) could work, but. I have a fairly strong preference for read_zipper_with_buffer because it establishes a pattern that could work with ZipperHead methods too. (_unchecked ZH methods are what MORK will really want, I assume)
As far as relocate, my issue is the case as with attach / ZipperBuffer::into_read_zipper It assumes a PathMap so we'd need to create a different API shape for a ZipperHead. And it is basically a convenience. But anyone who is looking for this API wants runtime performance over convenience.
I think the most important part not covered there is that you want to take these zippers at a path, you may not have access to the root, nor want to pay the cost of re-descending.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This introduces
ZipperBuffer. This allows re-using zipper path and stackVecs.Name up for change, don't have a strong preference.
detach+attach,relocatepreserve the path. might need to change the API so this is explicit.A few additional non-public API changes to make it work.
descend_to_byteused instead ofdescend_toto avoid aliased&mut.